Codecov Report

All modified lines are covered by tests ✅

Comparison is base (20f287f) 89.09% compared to head (0adf4ca) 89.05%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main    #2635      +/-   ##
==========================================
- Coverage   89.09%   89.05%   -0.05%     
==========================================
  Files         112      112              
  Lines       86932    86932              
  Branches    86932    86932              
==========================================
- Hits        77456    77419      -37     
- Misses       7246     7278      +32     
- Partials     2230     2235       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I'm not convinced this is the right approach - I think we should pick an approach that makes sense for our own splicing and go that way.

The intention here is to have a temporary solution without a performance regression until the splicing API changes are in place. We can work around this by caching multiple copies of InMemorySigner at a cost of some complexity and memory.

Yea, I get that, I'm just am a bit dubious of merging temporary solutions as they have a tendency to become permanent :). I'd much rather y'all just work on a fork for your immediate needs while we (or you, if you have free time) build a more sustainable solution.

Can this be closed now @devrandom?

Yes, but we still need a solution. Any thoughts?

Don't have any concrete suggestions as of yet.

Another issue here (not sure if it applies to VLS as well) is with our key derivation: our basepoints are derived in steps, based on the previous basepoint derived. This becomes problematic when we attempt to re-derive the signer for a channel that has already spliced. We need to communicate only the funding key has rotated somehow, perhaps via a splice counter.

right, I was thinking a splice counter or mix in the current funding outpoint

I see here two approaches:

• create Channel Signer::with_value() that clones the signer but lets you change the channel value
• pass the value into the four functions that need it

if we go with the second approach, do we remove it from the serialization format for the signer?

This should be resolved as part of #2743.

if we go with the second approach, do we remove it from the serialization format for the signer?

We've stopped deserializing signers since 0.113.0 and with 0.119.0 we won't allow downgrading past that.

pass the value into the four functions that need it

Apologies for the delay, I'd much prefer to go this route. The signing interface should basically be an interface which can be pasted onto the wire for LDK to communicate with VLS. If we pass the value (and funding txo) through to the relevant signing methods, I believe VLS would be able to operate just fine (it'd have to synthesize RBF/splice events based on detecting changes, but I think that's fine) with the interface when LDK adds support for splicing. Is that your understanding, or what else will VLS need?

I see here two approaches:

• create Channel Signer::with_value() that clones the channel but lets you change the channel value
  ...

note that I had a typo in my comment above

pass the value into the four functions that need it

Apologies for the delay, I'd much prefer to go this route. The signing interface should basically be an interface which can be pasted onto the wire for LDK to communicate with VLS. If we pass the value (and funding txo) through to the relevant signing methods, I believe VLS would be able to operate just fine (it'd have to synthesize RBF/splice events based on detecting changes, but I think that's fine) with the interface when LDK adds support for splicing. Is that your understanding, or what else will VLS need?

I think that's OK, with a couple of nits:

• I don't actually see why you'd pass the funding outpoint - that's already given in the CommitmentTransaction that's passed into the signing fns
• actually, maybe CommitmentTransaction is the best spot to add the funding value?
• I don't quite understand "it'd have to synthesize RBF/splice events based on detecting changes". we detect splices / candidates in our setup_channel (equivalent to your provide_channel_parameters). is that what you meant? or did you mean that we'd discover candidates when we are asked to sign the first commitment and the funding txs? that might be difficult, especially if our side doesn't contribute to the funding.

another thing to consider is that we'll have funding key rotation when splicing. the funding key seems more appropriate to provide in a repeated call to provide_channel_parameters

I don't actually see why you'd pass the funding outpoint - that's already given in the CommitmentTransaction that's passed into the signing fns

Right, sorry

actually, maybe CommitmentTransaction is the best spot to add the funding value?

Makes sense to me

I don't quite understand "it'd have to synthesize RBF/splice events based on detecting changes". we detect splices / candidates in our setup_channel (equivalent to your provide_channel_parameters). is that what you meant? or did you mean that we'd discover candidates when we are asked to sign the first commitment and the funding txs? that might be difficult, especially if our side doesn't contribute to the funding.

That's what I meant, yea. We could have some "splice_setup" call instead if that makes it easier, just trying to avoid redundant calls that we can elide.

another thing to consider is that we'll have funding key rotation when splicing. the funding key seems more appropriate to provide in a repeated call to provide_channel_parameters

Maybe that's a reason to have a setup_splice method, it would let the signer tell us about some new keys it wants us to use for a splice.

What does your API to CLN look like here?

so pretty similar to the LDK one. but the funding pubkeys are already provided (in both APIs). I'm just saying that the splice can be announced with the same provide_channel_parameters() call, it just needs to be repeated.

oh, but you are correct that we need to ask the signer for the next local funding pubkey, and that's a new call, or a change to provide_channel_parameters()

ISTM we should have a different call just because its clearer for the signer what's going on. Also, indeed, if we're asking for a new funding key we'll want to ask the signer. It seems like we're on the same page, do you have time to hack this up, or should we get an LDK contributor to (I know we have a few folks who want to work on splicing, so I'm sure there will be a volunteer somewhere).